Skip to content

dd: exit when bs/ibs/obs/cbs isn't positive#11630

Open
iburaky2 wants to merge 3 commits intouutils:mainfrom
iburaky2:fix/dd-bs-not-positive
Open

dd: exit when bs/ibs/obs/cbs isn't positive#11630
iburaky2 wants to merge 3 commits intouutils:mainfrom
iburaky2:fix/dd-bs-not-positive

Conversation

@iburaky2
Copy link
Copy Markdown
Contributor

@iburaky2 iburaky2 commented Apr 4, 2026

Match GNU coreutils behavior:

$ dd bs=0
dd: invalid number: '0'
$ dd ibs=0
dd: invalid number: '0'
$ dd obs=0
dd: invalid number: '0'
$ dd cbs=0
dd: invalid number: '0'

$ dd bs=-5
dd: invalid number: '-5'
$ dd ibs=-5
dd: invalid number: '-5'
$ dd obs=-5
dd: invalid number: '-5'
$ dd cbs=-5
dd: invalid number: '-5'

Currently:

Another option would be to check in parse_bytes / parse_bytes_with_opt_multiplier but I think it's better to exit earlier.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

GNU testsuite comparison:

Note: The gnu test tests/csplit/csplit-heap is now being skipped but was previously passing.
Note: The gnu test tests/dd/no-allocate is now being skipped but was previously passing.
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.

@oech3
Copy link
Copy Markdown
Contributor

oech3 commented Apr 4, 2026

Our dd already rejects negative value. So this PR might have duplicated logic with it.

@oech3

This comment was marked as resolved.

Sign in to view
@iburaky2 iburaky2 force-pushed the fix/dd-bs-not-positive branch from 6f89772 to 3fae276 Compare April 4, 2026 01:54
@iburaky2
Copy link
Copy Markdown
Contributor Author

iburaky2 commented Apr 4, 2026

mb I got the order wrong parse_bytes comes before validate(). Since parse_bytes is only used with those 4 parameters and all should reject 0 we can handle the check there.

Negative numbers are handled here:

fn parse_bytes_no_x(full: &str, s: &str) -> Result<u64, ParseError> {
    let parser = SizeParser {
        capital_b_bytes: true,
        no_empty_numeric: true,
        ..Default::default()
    };
    let (num, multiplier) = match (s.find('c'), s.rfind('w'), s.rfind('b')) {
        (None, None, None) => match parser.parse_u64(s) { // -5 can't be parsed as y64
            Ok(n) => (n, 1),
            Err(ParseSizeError::SizeTooBig(_)) => (u64::MAX, 1),
            Err(_) => return Err(ParseError::InvalidNumber(full.to_string())), // error here
        },
        (Some(i), None, None) => (parse_bytes_only(s, i)?, 1),
        (None, Some(i), None) => (parse_bytes_only(s, i)?, 2),
        (None, None, Some(i)) => (parse_bytes_only(s, i)?, 512),
        _ => return Err(ParseError::MultiplierStringParseFailure(full.to_string())),
    };
    num.checked_mul(multiplier)
        .ok_or_else(|| ParseError::MultiplierStringOverflow(full.to_string()))
}

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

GNU testsuite comparison:

Skip an intermittent issue tests/pr/bounded-memory (was skipped on 'main', now failing)

@sylvestre
Copy link
Copy Markdown
Contributor

sylvestre commented Apr 5, 2026

Thanks! Two things before this lands:

  1. The negative case doesn't actually match GNU. bs=-5 currently errors via try_into() => BsOutOfRange, which prints bs=N cannot fit into memory, not GNU's invalid number: '-5'. Could you collapse it into a single check? like:
    let num = parse_bytes_with_opt_multiplier(val)?;
    if num <= 0 {
    return Err(ParseError::InvalidNumber(val.to_string()));
    }
  2. Note val.to_string() (not num.to_string()) so inputs like 0x0 are echoed verbatim as GNU does.
  3. The test only checks exit code and empty stdout, so it doesn't actually verify the error message - which is the whole point of the PR. Please add a .stderr_contains("invalid number") so we catch regressions in the wording.

@iburaky2 iburaky2 force-pushed the fix/dd-bs-not-positive branch from 3fae276 to ef02de9 Compare April 6, 2026 02:08
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 6, 2026

GNU testsuite comparison:

Skip an intermittent issue tests/pr/bounded-memory (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/rm/isatty (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)

@iburaky2
Copy link
Copy Markdown
Contributor Author

iburaky2 commented Apr 6, 2026

I've fixed points 2 and 3 and also added 0x0 as a testcase

Regarding point 1, bs=-5 does match GNU's error message [see the passing test which now checks error message as proof :) ] because it's handled earlier in parse_bytes_no_x.

$ rust-gdb --args coreutils dd bs=-5
(gdb) b uu_dd::parseargs::parse_bytes_no_x
Breakpoint 1 at 0x1176ac4: file src/uu/dd/src/parseargs.rs, line 491.
(gdb) r
Breakpoint 1, uu_dd::parseargs::parse_bytes_no_x (full="-5", s="-5") at src/uu/dd/src/parseargs.rs:491
491	        ..Default::default()
(gdb) n
488	    let parser = SizeParser {
(gdb) 
493	    let (num, multiplier) = match (s.find('c'), s.rfind('w'), s.rfind('b')) {
(gdb) 
494	        (None, None, None) => match parser.parse_u64(s) {
(gdb) 
497	            Err(_) => return Err(ParseError::InvalidNumber(full.to_string())),
(gdb)

I'm not fully sure I fully understand what the point of

        num.try_into()
            .map_err(|_| ParseError::BsOutOfRange(arg.to_string()))

is. I guess this would only trigger if we were on a 32 bit system (since the function returns usize) and num (u64) was too large?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iburaky2 @oech3 @sylvestre